Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Repair code to avoid "instance variable not initialized" warning #12

Merged
merged 5 commits into from
Aug 19, 2019
Merged

Repair code to avoid "instance variable not initialized" warning #12

merged 5 commits into from
Aug 19, 2019

Conversation

martinstreicher
Copy link
Contributor

Ruby 2.6.2 generates the instance variable not initialized warning for a variable in this gem. I moved some code around to avoid the error message.

@coveralls
Copy link

coveralls commented Aug 16, 2019

Pull Request Test Coverage Report for Build 51

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 50: 0.0%
Covered Lines: 189
Relevant Lines: 189

💛 - Coveralls

@martinstreicher
Copy link
Contributor Author

No issues found by Rubocop. Specs are green.

@tycooon
Copy link
Owner

tycooon commented Aug 16, 2019

I guess you just partly reverted #10, maybe use defined? operator instead?

@martinstreicher
Copy link
Contributor Author

I will run a benchmark. I cannot imagine these more time, but let's find out.

.gitignore Outdated

# rspec failure tracking
.rspec_status

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like extra (unneeded) addition.

.gitignore Outdated
@@ -7,6 +7,8 @@
/spec/reports/
/tmp/
.ruby-version
.ruby-gemset
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to do this in separate PR or, at least, commit.

@AlexWayfer
Copy link
Contributor

Ruby 2.6.2 generates the instance variable not initialized warning for a variable in this gem.

How to get this?

Oh, I found: --warnings should be added into .rspec.

@AlexWayfer
Copy link
Contributor

Benchmark:

# frozen_string_literal: true

require 'bundler/setup'
Bundler.setup

require 'benchmark'
require 'benchmark/ips'
require 'benchmark/memory'


puts '```ruby'
puts File.read(__FILE__).gsub("\t", '  ')
puts '```'
puts
puts '### Output'
puts
puts '```'


require_relative 'lib/memery'
require_relative 'lib/memery_old'

class Foo
  class << self
    include Memery
    include MemeryOld

    def base_find(char)
      ('a'..'k').find { |letter| letter == char }
    end

    memoize_old def find_old(char)
      base_find(char)
    end

    memoize def find_new(char)
      base_find(char)
    end
  end
end


def memery_old
  Foo.find_old('d')
end

def memery_new
  Foo.find_new('d')
end


def test
  exit unless p (((p memery_old) == (p memery_new)))
end

test

Benchmark.ips do |x|
  x.report('memery_old') { memery_old }
  x.report('memery_new') { memery_new }

  x.compare!
end

Benchmark.memory do |x|
  x.report('memery_old') { 100.times { memery_old } }
  x.report('memery_new') { 100.times { memery_new } }

  x.compare!
end

puts '```'

Output

"d"
"d"
true
Warming up --------------------------------------
          memery_old    28.864k i/100ms
          memery_new    29.188k i/100ms
Calculating -------------------------------------
          memery_old    332.771k (± 4.5%) i/s -      1.674M in   5.041161s
          memery_new    341.168k (± 4.6%) i/s -      1.722M in   5.058746s

Comparison:
          memery_new:   341168.0 i/s
          memery_old:   332771.1 i/s - same-ish: difference falls within error

Calculating -------------------------------------
          memery_old    16.000k memsize (     0.000  retained)
                       400.000  objects (     0.000  retained)
                         3.000  strings (     0.000  retained)
          memery_new    16.000k memsize (     0.000  retained)
                       400.000  objects (     0.000  retained)
                         3.000  strings (     0.000  retained)

Comparison:
          memery_old:      16000 allocated
          memery_new:      16000 allocated - same

@tycooon
Copy link
Owner

tycooon commented Aug 16, 2019

I think we should add the rake benchmark task to this repo 🤔

@AlexWayfer
Copy link
Contributor

By the way, actual master version (with :ttl option) doesn't have such warnings.

@AlexWayfer
Copy link
Contributor

I think we should add the rake benchmark task to this repo thinking

I can try, but it will be without comparison, I guess. Or we need to get the library from master in some way, rename it, etc. …

Or we can just have an example of bench script (it's ignored for git at my comp now).

@tycooon
Copy link
Owner

tycooon commented Aug 16, 2019

I think that without a comparison is OK – you can always run it on master and your branch and compare manually.

@AlexWayfer
Copy link
Contributor

I think that without a comparison is OK – you can always run it on master and your branch and compare manually.

Done in #14.

* Add `:ttl` option for `memoize` method (#11)

Repeat calling original method after a specified time (in seconds).

* Add `--warnings` option for RSpec (#13)

* Add benchmark script (#14)

Can be executed by `bundle exec ruby benchmark.rb` or `bundle exec rake benchmark`.

* Repair code to avoid "instance variable not initialized" warning

* Revert previous change
@martinstreicher
Copy link
Contributor Author

Warming up --------------------------------------
         test_memery    28.370k i/100ms
Calculating -------------------------------------
         test_memery    302.473k (± 4.4%) i/s -      1.532M in   5.074632s
Calculating -------------------------------------
         test_memery    20.000k memsize (     0.000  retained)
                       500.000  objects (     0.000  retained)
                         3.000  strings (     0.000  retained)

@AlexWayfer
Copy link
Contributor

@martinstreicher, warnings are gone in the current master version. You can check it and close this PR.

@martinstreicher
Copy link
Contributor Author

I did check...

/Users/mss67/.rvm/gems/ruby-2.6.2@states-machines/bundler/gems/memery-7cb68b9295ed/lib/memery.rb:55: warning: instance variable @_memery_memoized_values not initialized
/Users/mss67/.rvm/gems/ruby-2.6.2@states-machines/bundler/gems/memery-7cb68b9295ed/lib/memery.rb:55: warning: instance variable @_memery_memoized_values not initialized
      finds models that must be advanced

The warning persists because line 55 is the same issue I identified previously.

@martinstreicher
Copy link
Contributor Author

My Gemfile...

gem 'memery', github: 'tycooon/memery', branch: 'master'

and Gemfile.lock

GIT
  remote: https://github.com/tycooon/memery.git
  revision: 7cb68b9295ed6eee1e02edc4640c61b775115593
  branch: master
  specs:
    memery (1.1.0)

@tycooon tycooon merged commit d16064a into tycooon:master Aug 19, 2019
@tycooon
Copy link
Owner

tycooon commented Aug 19, 2019

Thanks for the patch! I fixed a rubocop issue and merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants